Master add shortcuts matho #7536
Conversation
|
|
matho-odoo
force-pushed
the
master-add-shortcuts-matho
branch
6 times, most recently
from
0130936 to
945f5c2
Compare
old tests did not actually test the shortcuts functionality there was nothing asserted Task : 5231802
shift+F11 (new sheet), ctrl+enter (fill range) and alt+t (new table) Task: 5231802
matho-odoo
force-pushed
the
master-add-shortcuts-matho
branch
from
945f5c2 to
ba72476
Compare
hokolomopo
left a comment
hokolomopo
left a comment
There was a problem hiding this comment.
Hello 👋
Reading the spec of the task, you didn't do Insert row/column, was that discussed with francois ? And Insert Comment, but that's odoo specific.
Also the spec mentions showing the shortcuts in Odoo's ctrl+k menu, but I'm for doing it in another task.
| bottom: multipleRowsInSelection ? zone.top : zone.top - 1, | ||
| top: multipleRowsInSelection ? zone.top : zone.top - 1, | ||
| }; | ||
| this.originSheetId = this.getters.getActiveSheetId(); |
There was a problem hiding this comment.
allowDispatch should not change the state of the plugin. and this.originSheetId is not used here anyway
same comment for copy on left/copy on zone
| const { INSERT_TABLE } = require("../../src/actions/menu_items_actions"); | ||
| expect(INSERT_TABLE as jest.Mock).toHaveBeenCalled(); |
There was a problem hiding this comment.
I'd remove this (and the jest.mock("../../src/actions/menu_items_actions.ts" at top of the file), you already tested that a table was inserted.
In general avoid mocking imports unless absolutely necessary. It makes tests harder to understand, and breaks if we ever move files around.
Testing what the result of an action is is the superior option over testing expect(helper).toHaveBeenCalled() anyway. Testing that the helper has been called doesn't actually test much, the helper could be broken and not do anything, and the test wouldn't notice. Just test that a table was inserted, and it's enough 🙂
| }); | ||
|
|
||
| test("can copy and paste cell(s) on left using CTRL+R", async () => { | ||
| test("banane", () => { |
| // @ts-ignore | ||
| const notificationStore = env.__spreadsheet_stores__.get(NotificationStore); |
There was a problem hiding this comment.
Avoid ts-ignore when possible. I'm guessing env.getStore(NotificationStore) doesn't work because of the proxy ? Otherwise you could create a mock raiseError function and give it to the env (see raiseError tests in bottom_bar_component.test.ts)
| expect( | ||
| fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link']")?.textContent | ||
| fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link'] .o-menu-item-name") | ||
| ?.textContent | ||
| ).toBe("Insert link"); |
There was a problem hiding this comment.
nitpick: expect(".o-menu .o-menu-item[data-name='insert_link'] .o-menu-item-name").toHaveText("Insert link")
see our custom jest matchers in jest_extend.ts
| notificationStore.updateNotificationCallbacks({ | ||
| notifyUser: mockEnv.notifyUser || (() => {}), | ||
| raiseError: mockEnv.raiseError || (() => {}), | ||
| raiseError: mockEnv.raiseError || (jest.fn() as unknown as (message: string) => void), |
There was a problem hiding this comment.
As i said in a previous comment, pass a mocked raiseError to the env instead (mockEnv.raiseError)
3 participants
Description:
Add some shortcuts
Task: 5231802
review checklist